feat(desktop): add floating Bestie chat - #6908

Verdict: REQUEST CHANGES

Reviewed by :bot: Jude’s code review agent at exact head cc00a88e19e869a9a92ccfd52e515f5e6cd74069 against base 82051038a1e11cf17329f7325657625ab79b115c. Risk is high because this adds another live renderer for a real DM and therefore shares relay subscription, visible-channel, unread, app-shell, and composer contracts with the canonical channel surface.

Blocking findings

1. [P2] Preserve visible-channel ownership across concurrent channel surfaces. BestieChatPopover mounts useChannelSubscription(channel) (desktop/src/features/messages/ui/BestieChatPopover.tsx:50), while ChannelScreen independently mounts the same hook (desktop/src/features/channels/ui/ChannelScreen.tsx:212). Each hook writes the relay session's singleton visible-channel marker and unconditionally clears it during cleanup (desktop/src/features/messages/hooks.ts:396-405). Closing the popover can therefore set the marker to null while the main channel remains visible; the reverse unmount order can also erase the popover's marker. Concurrent views of the same DM additionally establish independent live subscriptions. Author action: make visible-channel/subscription ownership safe for concurrent consumers and add mount/unmount-order coverage proving one surface cannot clobber the other (including same-channel consumers).

2. [P2] Opening the Bestie surface bypasses canonical read consumption. The normal channel surface follows subscription/query setup with useChannelOpenReadState (desktop/src/features/channels/ui/ChannelScreen.tsx:206-231). The popover queries and subscribes (desktop/src/features/messages/ui/BestieChatPopover.tsx:50-51) but has no equivalent read-state transition. Reading a DM in this new surface therefore does not demonstrably consume its unread state. Author action: integrate the canonical read semantics and add a regression proving an unread Bestie DM clears when viewed in the popover.

3. [P2] A DM-open failure is presented as an empty real conversation with no in-surface recovery. The open failure only logs and emits a transient toast (desktop/src/features/messages/ui/BestieChatPopover.tsx:119-147). After the mutation settles, channel remains null, loading ends (:186-188), the surface renders “Your messages ... will show up here” (:250-268), and the composer stays disabled (:271-280). Offline, auth, or relay failure is thus mislabeled as an empty conversation and leaves keyboard/screen-reader users without Retry. Author action: render persistent failure + Retry, or close the popover after the toast; add a failure→retry→real-history regression.

4. [P2] The advertised app-level shortcut is unmounted while Settings displays it. The only keydown listener lives in BestieChatPopover (desktop/src/features/messages/ui/BestieChatPopover.tsx:149-169), but that component is nested under AppTopChrome, which is not mounted in Settings or huddle-room mode (desktop/src/app/AppShell.tsx:772-783). Settings still advertises ⌘1/Ctrl+1 when the gate is enabled (desktop/src/features/settings/ui/KeyboardShortcutsCard.tsx:36-40; desktop/src/shared/lib/keyboard-shortcuts.ts:100-107). Author action: move command ownership to an always-mounted, canonically gated app-shell layer and test it from Settings and ordinary composer focus; keep the visual trigger conditional.

Verification owner: author for fixes and biting regressions; reviewer for exact-head lifecycle/read-state review and rerun.

Validation at the reviewed head

• pnpm -C desktop test:e2e:bestie: PASS, 3/3 using the eligible E2E build; covers enabled sidebar, strict flag-off absence + inert Meta+1, and avatar/shortcut/send happy path.
• just desktop-test: PASS, 5,681/5,681.
• just desktop-check: PASS; three warnings and two informational diagnostics were outside this PR diff.
• git diff --check 82051038a1e11cf17329f7325657625ab79b115c..HEAD: PASS.
• Final exact SHA and tree: cc00a88e19e869a9a92ccfd52e515f5e6cd74069, clean.

One alternate direct Playwright invocation produced 1 pass / 2 enabled-path failures because it did not reproduce the package script's eligible build environment. The canonical repository command above passed, so I am not classifying that invocation mismatch as an author defect.

Dependencies and residual risk

The PR remains stacked on #6905 and blocked by #6902/#6903; those are dependency gates, not #6908 findings. No isolated native Tauri/real-relay journey, Windows Ctrl+1 observation, AX snapshot, or narrow-window/zoom/reduced-motion artifact was captured. Error/retry, Settings invocation, Escape/focus arbitration, and live incoming/read-state behavior are also absent from the current E2E. These are verification gaps owned by reviewer/tooling after the source-grounded defects are fixed; native-artifact absence is not itself the reason for this verdict.

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES
Reviewed: 54ca696dc20b2e3a9e26f8ac1888edd493fcae8a..095edcc01a24a7bc753316c2eff9520399b1adb5 (exact head 095edcc01a24a7bc753316c2eff9520399b1adb5)
Risk: high — a second live renderer for a real DM shares subscription, read-state, identity, keyboard-command, and composer contracts with the canonical channel surface.

The replacement source resolves the four prior blockers. Concurrent visible-channel and live-subscription ownership is reference-safe; the popover uses canonical read consumption; open failure presents a persistent alert with Retry; and the gated shortcut owner remains mounted across Settings/huddle while only its visual trigger depends on top chrome. Community/signer replacement remounts the scoped query/app subtree and pending open remains relay+signer bound. Independent local evidence passed Desktop tests 5,691/5,691, typecheck/check, canonical eligible Bestie E2E 6/6, plus focused huddle and composer-focus probes.

Blocking required-gate failure: the app-level shortcut regression is not platform-correct in canonical CI

Exact-head required job 98372141624 failed specifically in Bestie experiment e2e. The ordinary Desktop smoke step in that same job passed. desktop/tests/e2e/bestie-sidebar.spec.ts:96-122 hard-codes page.keyboard.press("Meta+1"); on the Linux runner, bestie-chat-popover never appeared after that keypress. The assertion failed on the initial attempt and both retries, producing 1 failed / 5 passed for the Bestie suite.

The product contract is ⌘1 on macOS and Ctrl+1 elsewhere. The checked-in canonical regression therefore does not exercise the platform's primary modifier and makes a required exact-head gate deterministically red. Whether the necessary correction is solely test modifier selection or also command handling, the PR cannot demonstrate its advertised cross-platform shortcut in its required CI shape.

Author action: make the Bestie E2E select the correct primary modifier for the execution platform and ensure Ctrl+1 is handled on Linux/Windows; rerun an exact-head required job to green. Keep coverage for Settings, focused composer, strict flag-off behavior, retry recovery, unread consumption, and stale community/signer fencing.

Verification owner: author for code/test correction; CI gate and reviewer for exact-head rerun confirmation.

Manual/native evidence: browser E2E exercised failure→retry, unread consumption, Settings/composer/huddle invocation, focus, Escape, and flag-off absence. No packaged native artifact, native AX snapshot, real-relay recovery run, or direct Windows observation was completed.

Residual risk: native/Windows/AX/zoom artifacts remain reviewer/tooling confidence gaps, not additional author work. A new head invalidates this verdict.

:bot: Jude’s code review agent — exact-head re-review of 06ad43e6c0c14e388ac14bc2718794ac6977ff05 against 9bea9aacccce509638173775b6eb36199875660b.

Blocking findings

P1 — protected Bestie implementation ships in the OSS artifact

desktop/src/app/AppShell.tsx:10,791 unconditionally imports and renders AppBestiePopover, while desktop/src/app/AppBestiePopover.tsx:1 statically imports BestieChatPopover. The runtime feature check in AppBestiePopover.tsx:5-6 cannot remove that protected module graph from the OSS bundle.

The repository's artifact oracle at desktop/scripts/build-protected-feature-artifacts.mjs:75-90 catches the leak. Exact-head CI run 33186202966 fails Desktop Core (98899889234), Desktop Windows Build (98899889286), and Desktop Build macOS (98899889174) with Official OSS desktop artifact contains protected Bestie/Chief content. A matching local just desktop-build also fails. This violates the explicit distribution boundary and blocks all three desktop build paths.

Author action: put the app-level implementation behind the build-selected @protected-features boundary (public selection exports a no-op with no static import path to the Bestie implementation; internal selection may import/render it), or an equivalent compile-time-selected boundary. Retain/add an artifact regression proving protected strings/modules are absent from the OSS selection, and restore the required Desktop Core/macOS/Windows jobs.

Verification owner: author for the patch and green jobs; reviewer for exact-head graph and artifact confirmation.

P2 — history-fetch failure is presented as a genuine empty conversation

useChannelMessagesQuery can reject when getChannelWindowEvents rejects (desktop/src/features/messages/hooks.ts:307-331). The popover models only openDmMutation failure: isLoading excludes messagesQuery.isError (desktop/src/features/bestie/BestieChatPopover.tsx:228-230), then rendering falls through to the empty-conversation message (:320-334) and enables the composer whenever a channel exists (:337-348). The current retry E2E injects only openDmErrors (desktop/tests/e2e/bestie-sidebar.spec.ts:158-189), so it does not protect this post-open failure.

If opening the DM succeeds but relay/history retrieval fails, existing history is falsely represented as empty and the user is invited to send into a degraded surface without a persistent explanation or recovery control.

Author action: render a persistent accessible error state for messagesQuery.isError, provide a bounded messagesQuery.refetch() recovery, and do not show empty/read-ready UI before successful settlement. Add a deterministic open-success → history-failure → Retry → restored-history regression.

Verification owner: author for UI and regression; reviewer for causal rerun and accessibility semantics.

Resolved former blockers

The exact-head source and focused tests support the new reference-counted visible-channel ownership, shared same-channel live subscription with stale-completion fencing, canonical read consumption while open, persistent open-DM error/Retry, always-mounted shortcut ownership, expected relay/signer routing, and request-generation/community fencing. Focused ownership/shortcut tests pass 7/7; the exact-head Bestie E2E and smoke/integration shards are green.

Exact-head validation

• PASS: remote head equality, clean tree, git diff --check, focused tests 7/7, just desktop-typecheck, and just desktop-check (pre-existing warnings/info outside changed files).
• PASS in CI: Bestie/smoke/integration and relay-backed E2E coverage.
• FAIL, PR-caused required gate: just desktop-build and Desktop Core/macOS/Windows CI artifact builds, as described above.
• Full local just desktop-test did not complete within the reviewer window during concurrent fresh dependency setup; exact-head Desktop Core passed its test phase before the artifact failure.

Native GUI/AX recording, real-relay failure→retry observation, direct Windows input observation, and zoom/reduced-motion/narrow-window artifacts remain confidence gaps, not additional author defects. Re-run affected gates and delta review on the replacement head.